-
Notifications
You must be signed in to change notification settings - Fork 99
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Transform an array into a buffer via Array.buffer
.
#387
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. 💯
), | ||
Suite.test( | ||
"buffer", | ||
Array.buffer<Nat>([0, 1, 2, 3]).toArray(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the type parameters needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm not sure. I copied the code (including the type parameters) from the prior test and adapted it. Will check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the type arguments are indeed required:
arrayTest.mo:222.5-222.31: type error [M0098], cannot implicitly instantiate function of type
<A>[A] -> Buffer<A>
to argument of type
[Nat]
because implicit instantiation of type parameter A is under-constrained with
Nat <: A <: Any
where
Nat =/= Any
so that explicit type instantiation is required
https://github.com/dfinity/motoko-base/runs/6870423809?check_suite_focus=true#step:8:373
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type instantiation is necessary because buffer is invariant (right?) so there is no principal choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add a doc comment and maybe consider adding a more general 'fromIter' method too, especially if we already have 'toIter'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me!
Maybe change the PR title (type signature?)
Array.buffer
Array.buffer
.
Hmm, shouldn't that be called toBuffer (or maybe even preferably Buffer.fromArray to avoid coupling Array.mo to Buffer.mo) for consistency with other conversions? And a (trivial) doc comment would be useful. It's not too late to change since we haven't released this yet. |
This PR is also related to #368. |
Drive-by: agreeing with Claudio, this should be Buffer.fromArray. |
To be clear, we are still talking about a module function within module If so, fine. Otherwise, this suggestion doesn't make sense to me. Update:Nevermind. I am looking at the other PR, and I think it's clear we are all talking about a module member, not a class member. #389 is ready for review. |
Creates a buffer from an array.